Apply features from the commandline first #3960
Conversation
There was a problem hiding this comment.
Nice! I think this is a good change to make. I do also think it would be good to pre-parse the target features section, too. Otherwise, once we depend on features during parsing, it won't be possible to parse correctly when a feature is in the target features section but not on the command line.
| @@ -3974,7 +3974,8 @@ BinaryenModuleRef BinaryenModuleRead(char* input, size_t inputSize) { | |||
| buffer.resize(inputSize); | |||
| std::copy_n(input, inputSize, buffer.begin()); | |||
| try { | |||
| WasmBinaryBuilder parser(*wasm, buffer); | |||
| // TODO: allow providing features in the C API | |||
| WasmBinaryBuilder parser(*wasm, FeatureSet::MVP, buffer); | |||
There was a problem hiding this comment.
| WasmBinaryBuilder parser(*wasm, FeatureSet::MVP, buffer); | |
| WasmBinaryBuilder parser(*wasm, FeatureSet::All, buffer); |
This is closer to the previous behavior. I would be surprised if it weren't also required by our tests.
There was a problem hiding this comment.
I'm not sure I follow you. Wouldn't this only be needed if we used to enable all the features in this code path? I don't see that. This should have no effect, that is, it sets the features to MVP, which is the default of a module anyhow.
But maybe I'm missing something... the tests on CI all failed due to lint, which is fixed now, so we can see what happens. Locally though they seem to pass for me.
There was a problem hiding this comment.
Oh right, never mind about the tests. I was thinking that the feature set here would control what feature would be able to be parsed, but that's not right. Why does this code have to change at all? FeatureSet::MVP is already the default feature set on a Module.
There was a problem hiding this comment.
This line changes only because I changed the API. I made the wasm binary builder class take a FeatureSet as a parameter. That feels nicer than the user setting the features on the module before in a separate operation. I could make this new parameter to the constructor default to MVP, and then this line could not change?
There was a problem hiding this comment.
But this capability isn't used anywhere except to set the features to MVP, which they already are, right?
There was a problem hiding this comment.
RoundTrip for example does apply the known correct features:
- WasmBinaryBuilder parser(*module, input);
+ WasmBinaryBuilder parser(*module, features, input);(And in wasm-io we set the features as well, although there we set them from the module, because that is all it has - a larger refactoring might also add an API there to get the features.)
| @@ -362,7 +362,8 @@ SExpressionWasmBuilder::SExpressionWasmBuilder(Module& wasm, | |||
| stringToBinary(str, size, data); | |||
| } | |||
| } | |||
| WasmBinaryBuilder binaryBuilder(wasm, data); | |||
| // TODO: support applying features here | |||
| WasmBinaryBuilder binaryBuilder(wasm, FeatureSet::MVP, data); | |||
There was a problem hiding this comment.
| WasmBinaryBuilder binaryBuilder(wasm, FeatureSet::MVP, data); | |
| WasmBinaryBuilder binaryBuilder(wasm, FeatureSet::All, data); |
We should leave disallowed features to be caught by validation.
There was a problem hiding this comment.
I don't understand you here either, sorry... How would enabling all features get us more validation in the text parser? (the text parser has no feature section parsing, but even if it did..?)
(tests passed)
There was a problem hiding this comment.
I was thinking that we would want to be able to parse all features by default, but the feature set passed here won't actually control what features can be parsed, so I was confused. Disregard this.
There was a problem hiding this comment.
Actually, why does this need to change to apply a new feature set? Can't it just keep the default one that wasm already had?
There was a problem hiding this comment.
(same issue as above, I think)
|
Agreed about pre-parsing the feature section, I think we will need that eventually (when we have a case that parsing is affected by a feature, and the feature is in the feature section - which might happen if someone starts to use the features section with gc-nn-locals...). |
There was a problem hiding this comment.
Makes sense. Thanks for your patience!
Oh, actually #3960 (comment)
The features section is additive since #3960. For the fuzzer to know which features are used, it therefore needs to also scan the features section. To do this, run --print-features to get the total features used from both flags + the features section. A result of this is that we now have a list of enabled features instead of "enable all, then disable". This is actually clearer I think, but it does require inverting the logic in some places.
As suggested in
#3955 (comment)
This applies commandline features first. If the features section is present, and
disallows some of them, then we warn. Otherwise, the features can combine
(for example, a wasm may enable feature X because it has to use it, and a user
can simply add the flag for feature Y if they want the optimizer to try to use it;
both flags will then be enabled).
This is important because in some cases we need to know the features before
parsing the wasm, in the case that the wasm does not use the features section.
In particular, non-nullable GC locals have an effect during parsing. (Typed
function references also does, but we found a way to apply its effect all the time,
that is, always use the refined type, and that happened to not break the case
where the feature is disabled - but such a workaround is not possible with
non-nullable locals.)
To make this less error-prone, add a FeatureSet input as a parameter to
WasmBinaryBuilder. That is, when building a module, we must give it the
features to use while doing so.
This will unblock #3955 . That PR will also add a test for the actual usage
of a feature during loading (the test can only be added there, after that PR
unbreaks things).